I think this comment is slightly misleading and this test is completely clear without the comment.

Thank you, the correct term should be return bytearray to Python. I think one-way is true that bytearray has to form explicitly with py::bytearray similarly as py::bytes? I can correct that or do you think both bytes/bytearray tests are self-explainable?

This was a PR, not an Issue. I think it's better to delete this, too. We have the git history with full context.

Could you please add tests with empty bytearrays?

And with malformed utf-8, to prove that there really isn't any encoding (see e.g. malformed_utf8 in test_pytypes.py).

I looked at the Python C code underneath PyByteArray_AsString, it special-cases empty arrays. But in any case, tests for corner cases are best practice.

Sure, an empty bytearray test case will be added.
For malformed_utf8, I'm not sure it's the same case as casting to python str. In test_pytypes, it's casting a single byte to str, and str is not fulfilled with a utf-8 resulting "b'\x80'". While passing b"\x80" to C++ string it would be treated as a byte and has size 1. Is this the case you're looking for?

The motivation/idea is very simple and high-level:

• We want to be sure this works for malformed utf-8.

We know the current implementation does. We want to be sure that's not accidentally broken somehow (e.g. refactoring).

My suggestion was based on past experience, where that actually happened.

I see your point, the test is added. I'm trying to make sure assert m.string_length(bytearray(b"\x80")) == 1 is the behavior we're looking for 😊

Wouldn't it just be better to parameterize the bytes_to_str tests and have them be rerun with all the bytes args wrapped as bytearrays(via pytest fixture for instance? or by having a function that either returns the bytes or the bytes wrapped in a bytearray depending on test parameters)

Hi @Skylion007, I also came up wrapping up the params for better coverage. Considering there're at least 3 dimensions (bytearray vs bytes, string_lenth vs strlen, bytearray with/without params), and a few corner cases(empty bytearray), I would suggest the current intuitive version should give us similar coverage with simplicity. What do you think?

Sounds good.

I believe (!) this, and the existing code above, are a bit awkward, because bytes will always be true in this context, or something bad happened upstream (e.g. a bug in an alpha release of a new Python version), and this code will silently ignore it. Also, assuming bytes could actually be legitimately false, do we need to call PyErr_Clear()?

Maybe:

    if (!bytes) pybind11_fail("Unexpected PyByteArray_AsString() failure.");

Or just remove the if and let it segfault? Almost seems best.

I'm uncertain if there will be a case bytes is nullptr/NULL. I agree with you we better shout out for such an error.
I'll wait for more comments for this one.

I see you added the checks, thank you!

This PR looks great to me. The only reason I'm holding off clicking the approve button is that I want to run this through our (Google's) global testing. It will probably take me a couple days to restore my ability to do that; I need to merge master into the smart_holder branch first, which will take some effort after the sweeping clang-format changes.

I'll get back here asap.